Fix invalidations from novel Integer conversions #36459

timholy · 2020-06-27T21:31:14Z

Defining

    struct MyInt <: Integer
        x::Int
    end
    (::Type{T})(x::MyInt) where T<:Integer = T(x.x)

triggers about 830 unique method invalidations. This fixes the majority of them, and most of the rest are nothing to worry about (they are in Core.Compiler, or run in the normalize_keymap phase of REPL initialization). One package that adds this kind of method is GeometryTypes, which is used by Plots. This PR, together with JuliaWeb/HTTP.jl#549, fix the last "big" sources of invalidation from using Plots. Altogether there are "only" 440 invalidations left, many of which are probably unconcerning; when I started, my estimate (which is probably ballpark but not entirely comparable due to some changes in how I'm counting) was nearly 10-fold higher (see https://github.com/JuliaLang/www.julialang.org/pull/794/files).

EDIT: fun fact, the opposite case (#29166) was handled in #30830.

This PR by itself is not a huge effect, but for the record:

Just the predecessors of this PR (which is better than current master), including my customizations of #35877:

julia> @time using Plots; @time p = plot(rand(10)); @time display(p)
  5.339177 seconds (9.02 M allocations: 561.481 MiB, 4.25% gc time)
  1.910392 seconds (2.81 M allocations: 156.618 MiB, 1.55% gc time)
  4.631717 seconds (6.53 M allocations: 364.880 MiB, 1.48% gc time)

Adding this PR on top of the previous:

julia> @time using Plots; @time p = plot(rand(10)); @time display(p)
  4.853647 seconds (9.23 M allocations: 571.112 MiB, 3.34% gc time)
  1.898238 seconds (2.81 M allocations: 156.582 MiB, 3.03% gc time)
  4.382853 seconds (6.46 M allocations: 361.079 MiB, 1.48% gc time)

Compared to the timings I posted here for master, this is almost 2s faster, so the cumulative effect is mounting (but also coming to its end, I don't expect much further progress just from fixing invalidations). There's still about 0.8s of precompilation, so getting more to precompile would add a little more benefit.

However, this specific PR should not be merged immediately. For most of my invalidation PRs, I think the improvements in inference can also be justified as improving code-quality. While that applies to some of this PR, a lot of it is just type-annotation, and one might really wish that most of those annotations aren't necessary. Consequently I've marked this RFC because it's worth asking whether there is a more systematic way to achieve the same benefits without all the annotation.

timholy · 2020-06-27T23:14:45Z

One thought (essentially an alternative to #35901 and the suggestion to use runtime dispatch for any non-specialized argument) would be to be able to annotate methods like this:

@ifabstract Int ncodeunits(s::AbstractString) = #= implementation=#

The idea here is that specific methods can override the return type, but that the supplied type is used if inference is purely abstract, i.e., using just the constraints in the method definition itself. And then inference would use this for methods that called the abstract definition.

kimikage

In general, I don't like "whack-a-mole" type assertions.:confused:

This is off-topic, but there seems to be room for optimization in integer-->string conversions(e.g. bin). So I think it's better to tackle the conversion functions on another PR.
Edit:
I submitted PR #36470, which only targets bin() and so on.

base/int.jl

@@ -382,7 +382,7 @@ julia> leading_zeros(Int32(1))
 31
 ```
 """
-leading_zeros(x::BitInteger) = ctlz_int(x) % Int
+leading_zeros(x::BitInteger) = (ctlz_int(x) % Int)::Int


base/intfuncs.jl

+        @inbounds a[i] = (0x30+(x&0x1)::Unsigned)::Unsigned
+        x = (x >> 1)::Unsigned


timholy · 2020-06-29T12:09:04Z

In general, I don't like "whack-a-mole" type assertions.confused

I'm not a fan either, which is why this PR feels ugly compared to most of the former ones. The issue is that these are code paths that end up getting used by callers with poor inference (sometimes due to @nospecialize, meaning that the poor inference is deliberate). These annotations "snip" call chains at strategic points that would otherwise lead to the invalidation of hundreds of methods, which is why it may be justified despite the ugliness. Nevertheless, if we can find a more systematic way (which is essentially what #36463 is about) I'd prefer it.

KristofferC · 2020-06-29T12:30:16Z

I think it is perfectly fine to live with a bit of ugliness to get rid of the most egregious invalidations (based on empirical data). When the low hanging fruits are gone, then we are in a better position to evaluate if a more systematic method is needed or if it is better to focus on another angle for attacking invalidations etc.

I'm a heavy proponent of not letting perfect be the enemy of good.

JeffBezanson · 2020-06-29T22:01:17Z

base/regex.jl

-iterate(s::SubstitutionString, i::Integer...) = iterate(s.string, i...)
+ncodeunits(s::SubstitutionString) = ncodeunits(s.string)::Int
+codeunit(s::SubstitutionString) = codeunit(s.string)::Type{<:Union{UInt8, UInt16, UInt32}}
+codeunit(s::SubstitutionString, i::Integer) = codeunit(s.string, i)::Union{UInt8, UInt16, UInt32}


I'm pretty sure this is true for all string types in practice, but it's not technically required.

Docs say this:

codeunit(s::AbstractString) -> Type{<:Union{UInt8, UInt16, UInt32}}

I've been taking the docs as license to enforce stuff. Should I not?

I guess someone could have a different code unit, but it would be very weird. Not sure all of the logic in base would work correctly with some other code unit.

Defining struct MyInt <: Integer x::Int end (::Type{T})(x::MyInt) where T<:Integer = T(x.x) triggers about 830 unique method invalidations. This fixes the majority of them, but it's a lot of type-annotation.

timholy · 2020-08-03T11:40:50Z

I've stripped out the part that is now being handled by #36470. If there are no objections, I'll merge this fairly soon.

timholy force-pushed the teh/integer branch from 1b3f78f to cd3434d Compare June 27, 2020 23:00

timholy force-pushed the teh/integer branch 2 times, most recently from a53e6d5 to 06b86f7 Compare June 28, 2020 07:18

kimikage reviewed Jun 28, 2020

View reviewed changes

kimikage mentioned this pull request Jun 29, 2020

Optimize integer-->string conversions #36470

Merged

JeffBezanson reviewed Jun 29, 2020

View reviewed changes

timholy mentioned this pull request Jul 1, 2020

[AUTO] Update precompiles JuliaPlots/Plots.jl#2835

Merged

timholy force-pushed the teh/integer branch from 55922cf to 66f4375 Compare August 2, 2020 20:31

timholy changed the title ~~RFC: fix invalidations from novel Integer conversions~~ Fix invalidations from novel Integer conversions Aug 2, 2020

timholy added the compiler:latency Compiler latency label Aug 2, 2020

Fix invalidations from novel Integer conversions

374c8d4

Defining struct MyInt <: Integer x::Int end (::Type{T})(x::MyInt) where T<:Integer = T(x.x) triggers about 830 unique method invalidations. This fixes the majority of them, but it's a lot of type-annotation.

timholy force-pushed the teh/integer branch from 66f4375 to 374c8d4 Compare August 3, 2020 00:42

timholy merged commit a6de8b6 into master Aug 5, 2020

timholy deleted the teh/integer branch August 5, 2020 14:09

KristofferC mentioned this pull request Aug 27, 2020

fix markdown rendering #37235

Merged

timholy mentioned this pull request Sep 26, 2020

Fixes for non-Int based lengths #37741

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix invalidations from novel Integer conversions #36459

Fix invalidations from novel Integer conversions #36459

timholy commented Jun 27, 2020 •

edited

Loading

timholy commented Jun 27, 2020 •

edited

Loading

kimikage left a comment •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as outdated.

timholy commented Jun 29, 2020 •

edited

Loading

KristofferC commented Jun 29, 2020

JeffBezanson Jun 29, 2020

timholy Jun 29, 2020

StefanKarpinski Jun 29, 2020

timholy commented Aug 3, 2020

		@inbounds a[i] = (0x30+(x&0x1)::Unsigned)::Unsigned
		x = (x >> 1)::Unsigned

Fix invalidations from novel Integer conversions #36459

Fix invalidations from novel Integer conversions #36459

Conversation

timholy commented Jun 27, 2020 • edited Loading

timholy commented Jun 27, 2020 • edited Loading

kimikage left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as outdated.

timholy commented Jun 29, 2020 • edited Loading

KristofferC commented Jun 29, 2020

JeffBezanson Jun 29, 2020

Choose a reason for hiding this comment

timholy Jun 29, 2020

Choose a reason for hiding this comment

StefanKarpinski Jun 29, 2020

Choose a reason for hiding this comment

timholy commented Aug 3, 2020

timholy commented Jun 27, 2020 •

edited

Loading

timholy commented Jun 27, 2020 •

edited

Loading

kimikage left a comment •

edited

Loading

timholy commented Jun 29, 2020 •

edited

Loading